[RNMobile] Embed Block Bottom Sheet to set a link #31343
Conversation
ceyhun
added
the
Mobile App - i.e. Android or iOS
|
Size Change: -283 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
# Conflicts: # packages/block-library/src/embed/embed-placeholder.native.js
When the attributes are modified in the LinkSettingsNavigation component the setAttributes callback is triggered. This callback is called once unlike the onClose callback, which is called multiple times and doesn't guarantee that the attributes are up-to-date at the time is triggered.
|
👋 @hypest The PR is ready for review, I've updated it with the latest changes from |
There was a problem hiding this comment.
Thanks for the updates @fluiddot and good job with the initial implementation @ceyhun!
I made a first review pass and left some comments. Generally looks good to me but I'm not sure yet about state/data updates. I probably need another, deeper pass to check if there's an issue with the bottom sheets in general or it's something specific to the implementation on this PR. Let me know if your thoughts in the comments.
Thanks @hypest for the review! I'll review further the state/data updates logic and provide further information about how it works and if the approach we're following here is reliable for the purpose of this block. |
I noticed that if a onDismiss prop is passed to the bottom sheet component, the internal function onDismiss is not executed. This behavior looks wrong because this function is intended to actually call the onDismiss prop.
|
@hypest I investigated deeper the state/data updates and I spotted some cases that weren't working properly, so I addressed them (they're posted as comments in the file changes) and refactored a bit the PR for this purpose. Being this said, the PR is ready for a new review, I'd appreciate it if you could take a look, thanks 🙇 ! |
|
I'm still in the process of reviewing the new changes @fluiddot , but wanted to surface something I noticed in the meantime: Looks like the BlockSheet onDismiss() is called when opening the Inserter, which feels weird/wrong. That's not related to the embed block at all, but haven't tried this yet on EDIT: |
True, there's a bottom sheet (code reference) that is created along with other UI elements when the editor is mounted. I had the same issue time ago when I was debugging an issue related to the reusable blocks 😄 . |
|
✅ Add the Embed block For the future additional work on the block, I think it would be nice if the "Invalid URL" is also displayed on the block itself somehow. |
We should probably add in the PR description that indeed we haven't implemented yet re-opening in the non-empty URL case, wdyt @fluiddot ? |
Is that feature related to this item from the referenced issue?
If so, |
|
Ah, sorry, I wasn't referring the the toolbar button which is indeed referenced in the Missing Features, but rather, to the fact that the current placeholder (when the URL is set) is a text field that doesn't support tapping to edit/replace. I know that in the code and the PR comments that's clear, but I'm again imagining the future reader of the PR and we should message them that this PR doesn't support editing. Not a biggie though, we might as well just rephrase the reference to the pencil feature with something like "Editing is not supported yet and will be enabled when we'll implement the ...". WDYT? |
Oh ok, I get your point, it's true that once the URL is set it doesn't allow to replace it. I didn't thought about that because this feature is still under development but since we're going to merge it soon it's better to detail what's missing 👍 . |
There was a problem hiding this comment.
LGTM! Awesome work here @ceyhun and @fluiddot!
Thanks for picking this PR up and updating it @fluiddot while @ceyhun is on vacation. That way we can move the Embed port effort forward. Also, apologies for being a bit slow with the review from my side, or going down some darker corners a tad too much 🤗.
I think the implementation in this PR has turned out to be a bit complex, possibly because we are reusing most of embed/edit.js (a good approach anyway), and took me longer to wrap my head around it. I hope we left enough breadcrumbs around for our future selves to understand what's going on. We can always revisit and and more if needed 👍
Let's merge this!
Description
This PR implements the Embed Block Bottom Sheet to set a link.
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3436
Use of
onCloseandonDismisscallbacks from the bottom sheet componentIt's not very clear the difference between the
onCloseandonDismisscallbacks, so for the purpose of this PR, I'd like to briefly describe when these callbacks are triggered and how they're used in the embed sheet.onCloseThis callback is triggered by the actions listed below, as far as I checked, looks like the callback is intended for requesting the parent to handle the close flow.
One thing I'd like to note is that this being called multiple times in some cases, I'm not sure if this behavior is intended or not, so it would be nice that we investigate it deeper in the future. As an example, when tapping the backdrop element when the modal is visible, the callback is triggered a first time, and then right after, as the modal is dismissed, it's triggered for a second time.
This is caused by the
onDismissfunction from the component that for some reason, callsonCloseBottomSheetthat triggers theonClosecallback:gutenberg/packages/components/src/mobile/bottom-sheet/index.native.js
Lines 310 to 318 in 32c4466
gutenberg/packages/components/src/mobile/bottom-sheet/index.native.js
Lines 340 to 351 in 32c4466
Actions that trigger
onClosecallback:onBackdropPresscallback).onBackButtonPresscallback).onSwipeComplete callback).onAccessibilityEscapecallback).onDismissThis is triggered when the modal has been already dismissed/hidden, which only happens once in the closing flow.
How are they used in the embed sheet
onClose: As is intended for hiding the modal, we're using it just to update the internal state of the placeholder component and set the visibility value tofalse. As mentioned earlier, this can be triggered multiple times but since we're only updating the internal state this is not an issue.gutenberg/packages/block-library/src/embed/embed-placeholder.native.js
Line 80 in 32c4466
onDismiss: It's being used for handling the URL attribute update. The reason for using this callback instead ofonCloseis because we have the guarantee that will be called only once.gutenberg/packages/block-library/src/embed/embed-bottom-sheet.native.js
Lines 36 to 46 in 32c4466
How has this been tested?
Missing features
Can re-open by pressing on the embed block placeholder.Depends on [Embed block] Implement Generic Embed Block placeholder UI wordpress-mobile/gutenberg-mobile#3273.
Depends on [Embed block] Implement specific Embed preview wordpress-mobile/gutenberg-mobile#3276
Screenshots
embed-bottom-sheet.mp4
embed-block-android.mp4
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).